Reviewer's Guide

Refactors XInput property and button mapping handling to correctly size, copy, and free C-allocated buffers, switching to length-aware APIs and GoBytes, and adding a dedicated C-side free helper to prevent leaks.

Sequence diagram for updated GetProperty data and memory handling

sequenceDiagram
    participant GoWrapper as Go_GetProperty
    participant CLayer as C_get_prop
    participant X11 as XInput_XIGetProperty
    participant CFree as C_free_prop_data

    GoWrapper->>CLayer: get_prop(id, prop, &nbytes)
    activate CLayer
    CLayer->>X11: XIGetProperty(length=0)
    X11-->>CLayer: act_type, act_format, num_items, bytes_after, data
    CLayer->>CLayer: check ret and act_type
    CLayer->>CLayer: XFree(data) if not null
    CLayer->>CLayer: if bytes_after == 0 return NULL
    CLayer->>X11: XIGetProperty(length=(bytes_after+3)/4)
    X11-->>CLayer: act_type, act_format, num_items, bytes_after, data
    CLayer->>CLayer: compute nbytes = num_items * (act_format/8)
    CLayer-->>GoWrapper: data pointer, nbytes
    deactivate CLayer

    GoWrapper->>GoWrapper: datas = C.GoBytes(data, nbytes)
    GoWrapper->>CFree: free_prop_data(data)
    CFree-->>GoWrapper: 
    GoWrapper-->>GoWrapper: return datas, int32(nbytes)

Sequence diagram for updated GetButtonMap data copying

sequenceDiagram
    participant GoWrapper as Go_GetButtonMap
    participant CLayer as C_get_button_map

    GoWrapper->>CLayer: get_button_map(xid, devName, &cbtnNum)
    CLayer-->>GoWrapper: cbtnMap pointer, cbtnNum
    GoWrapper->>GoWrapper: btnMap = C.GoBytes(cbtnMap, cbtnNum)
    GoWrapper->>CLayer: C.free(cbtnMap)
    CLayer-->>GoWrapper: 
    GoWrapper-->>GoWrapper: return btnMap, nil

File-Level Changes

deepin pr auto review

这份代码修改主要涉及 X11 输入设备属性的获取与处理，包括 C 语言底层实现和 Go 语言封装层。以下是对代码的审查意见：

1. 语法逻辑

优点：

• C 代码 (property.c)：get_prop 函数重构后逻辑更严谨。采用了两步获取策略（先查询大小，再获取数据），避免了固定缓冲区大小限制，能够正确处理任意长度的属性数据。
• Go 代码 (utils.go)：getInt8Prop, getInt16Prop, getInt32Prop, getFloat32Prop 现在基于字节数 (nBytes) 进行校验，逻辑更加准确。例如，对于 int16，检查字节数是否能被 2 整除，以及字节数除以 2 是否等于预期的项目数。这比之前直接比较 num != nitems 更能反映底层数据的实际布局。
• Go 代码 (wrapper.go)：移除了 ucharArrayToByte 函数，改用标准库的 C.GoBytes，减少了手动内存操作的代码，降低了出错风险。

改进建议：

• C 代码 (property.c)：在 get_prop 中，XCloseDisplay(disp) 的调用位置在成功路径和失败路径中重复出现。虽然在当前的 pthread_mutex 锁定范围内是必须的，但可以考虑使用 defer 风格的清理（C 语言中可用 goto cleanup 模式）来集中处理资源释放，减少代码冗余和遗漏 XCloseDisplay 的风险。

  // 示例：使用 goto cleanup 模式
  unsigned char* get_prop(...) {
      // ... 初始化 ...
      pthread_mutex_lock(&x11_global_mutex);
      // ... 获取 display ...
      
      // Step 1
      ret = XIGetProperty(...);
      if (ret != Success || ...) {
          goto cleanup; // 跳转到清理
      }
      // ... Step 2 ...
      
  cleanup:
      if (data) XFree(data);
      if (disp) XCloseDisplay(disp);
      pthread_mutex_unlock(&x11_global_mutex);
      return NULL;
  }

• Go 代码 (wacom.go)：doAction 函数中 return errors.New(string(out)) 虽然语法正确，但如果 out 包含换行符或大量文本，作为错误信息可能不够友好。建议对 out 进行简单的 strings.TrimSpace 处理。

  if err != nil {
      return errors.New(strings.TrimSpace(string(out)))
  }

2. 代码质量

优点：

• 错误处理：Go 层增加了详细的错误信息，例如 "property data is empty" 或 "byte count ... is not divisible by ..."，极大地提高了调试效率。
• 资源管理：在 wrapper.go 的 GetProperty 中，引入了 defer C.free_prop_data(cdatas)，确保 C 层分配的内存（由 XIGetProperty 返回）能够被正确释放，防止内存泄漏。同时 button_map.go 中也改用了 C.GoBytes，配合 defer C.free，资源管理更清晰。
• 注释：C 代码中增加了注释说明了 get_prop 的返回值类型和调用者责任（需调用 XFree），Go 代码中也增加了关于字节大小计算的注释，提高了可读性。

改进建议：

• C 代码 (property.c)：free_prop_data 是一个很好的封装，但建议在头文件 (property.h) 的注释中明确说明它是 XFree 的包装器，以便其他开发者理解其行为。
• Go 代码 (utils.go)：getInt8Prop 等函数中的校验逻辑有重复模式（检查空、检查整除、检查数量）。可以考虑封装一个通用的辅助函数来处理这些检查，减少重复代码。

  func checkPropBytes(id int32, prop string, datas []byte, nBytes int32, itemSize int32) error {
      if len(datas) == 0 {
          return fmt.Errorf("Get prop '%v -- %s' failed: property data is empty", id, prop)
      }
      if nBytes%itemSize != 0 {
          return fmt.Errorf("Get prop '%v -- %s' failed: byte count %d is not divisible by %d", 
              id, prop, nBytes, itemSize)
      }
      if nBytes/itemSize != nBytes/itemSize { // 这里逻辑需根据实际调用调整，仅作示例
          // ...
      }
      return nil
  }

3. 代码性能

优点：

• C 代码 (property.c)：去掉了 MAX_BUF_LEN 限制，不再需要预先分配大块内存或担心数据截断。虽然引入了两次 X11 调用，但对于大多数属性操作，性能影响可以忽略不计，且换取了正确性和安全性。
• Go 代码 (wrapper.go)：使用 C.GoBytes 替代了手动循环拷贝 ucharArrayToByte。C.GoBytes 是运行时提供的优化函数，通常比手动 Go 循环遍历 C 数组性能更好。

改进建议：

• Go 代码 (wrapper.go)：在 GetProperty 中，C.GoBytes 会进行一次内存分配和拷贝。如果上层调用者（如 getInt8Prop）只是读取数据而不需要修改，可以考虑返回一个切片，该切片底层引用 C 内存（通过 unsafe.Pointer），但这需要极其小心地管理生命周期（必须在 C 内存被 XFree 之前使用完）。鉴于当前代码已经使用了 defer 释放 C 内存，保持现状（拷贝数据）是更安全且性能足够的选择。

4. 代码安全

优点：

• C 代码 (property.c)：
  • 增加了对 nbytes 指针的空指针检查。
  • 增加了对 bytes_after 的检查，处理了属性存在但无数据的情况。
  • 关键改进：添加了整数溢出检查 if (nbytes_ul > INT_MAX)，防止 unsigned long 类型的字节数在转换为 int 时发生截断或溢出，这是非常重要的安全加固。
  • 增加了 free_prop_data 函数，封装了 XFree，为 Go 层提供了安全的内存释放接口。
• Go 代码 (wrapper.go)：
  • 使用 defer 确保 C.free_prop_data 被调用，即使发生 panic 也能释放 C 层内存，防止内存泄漏。
  • GetProperty 返回的字节数 nbytes 现在直接来自 C 层计算，而不是固定的 maxBufferLen，避免了越界读取风险。
• Go 代码 (utils.go)：
  • 严格的类型和长度检查（如 nBytes%2 != 0）防止了读取格式错误的数据导致 panic。

改进建议：

• C 代码 (property.c)：在 get_prop 中，act_format 来自 X11 服务器，理论上应该是 8, 16, 或 32。但为了防御性编程，可以增加对 act_format 的检查，如果它不是这三个值之一，应视为错误并清理资源。

  if (act_format != 8 && act_format != 16 && act_format != 32) {
      fprintf(stderr, "[get_prop] Unsupported format %d for %s\n", act_format, prop);
      goto cleanup;
  }

• Go 代码 (wrapper.go)：C.GoBytes 接受的长度是 C.int。虽然 C 层已经检查了 nbytes <= INT_MAX，但在 Go 侧再次断言一下 nbytes 的范围也无妨，增加一层防御。

  if nbytes < 0 || int(nbytes) > 1<<31-1 { // C.int 通常是 32 位
      return nil, 0
  }

总结

这次代码修改在安全性和健壮性方面有显著提升，特别是 C 层对内存管理和整数溢出的处理，以及 Go 层对数据校验的加强。性能方面也有小幅优化（使用 C.GoBytes）。主要改进点集中在去除硬编码限制、增加错误细节和完善资源释放上。

建议采纳上述关于 C 代码 goto cleanup 模式、act_format 检查以及 Go 代码错误信息格式化的建议，以进一步提升代码的健壮性和可维护性。

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: electricface, fly602

The full list of commands accepted by this bot can be found here.